Skip to content

perf(repsel): element-shape loop clone serves the element-binding form through function boundaries (#7766) - #7778

Merged
proggeramlug merged 6 commits into
mainfrom
repsel/7766-param-array-element-shape
Aug 10, 2026
Merged

perf(repsel): element-shape loop clone serves the element-binding form through function boundaries (#7766)#7778
proggeramlug merged 6 commits into
mainfrom
repsel/7766-param-array-element-shape

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes #7766.

The verified framing (the issue asked for this first)

The issue's mechanism 1 said the clone "is admitted but its body does not specialize element field reads" and asked that framing to be verified before building on it. Measured on current main (423bb44):

  • The direct spelling is already served. s += ps[i].x + ps[i].y through a typed P[] parameter gets a versioned clone whose deref block cond_brs INTO the fast preheader (the perf(repsel): the element-shape loop clone fires on arr.length and aliased element types (#7480) #7701 entry test), with offset loads and no calls in the fast body. Runtime confirms: 0.10s vs node 0.07s on 200k×200 (dev profile). The by-name/guard/coerce calls the issue counted in (A)'s IR belong to the slow clone — mandatory fallback IR, dead at runtime for an honest caller.
  • The binding spelling was the real gap. { const r = ps[i]; s += r.x + r.y } — and for (const p of ps), which desugars to exactly that shape — got no clone at all: 2.2× and 7.8× slower than the direct spelling respectively. Two independent causes:
    1. the matcher admitted only a single-statement accumulator body, and
    2. the for-of desugar mints its counter as Number(0.0), which collect_integer_let_ids does not seed, so the desugared counter never joined integer_locals, never got a canonical i32 slot, and every i32-counter loop optimization silently declined the for…of spelling of loops it served in indexed form.

Mechanism decision (all three evaluated)

1. Extend the element-shape guarded clone — chosen. The guard establishes the fact at runtime (js_array_ensure_element_shape verifies every element's class; the declared type only names the class id to check against), so it is sound for parameters by construction — exactly the "establish, don't assume" bar the issue sets. With the framing corrected, the remaining work was matcher coverage, not read specialization, which makes this by far the smallest correct change (~120 lines, no new ABI, one clone per loop as today).

2. Clone-and-route at the call site — needs per-signature callee clones (code size), call-site proof plumbing across modules, and serves only calls whose arguments already carry proofs — which #7170 measured as rare in dependency JS (91.6% of its rule-1 population sits in closures where no proof exists). A runtime guard is still needed for unproven callers, i.e. mechanism 1 is a prerequisite of 2, not an alternative.

3. Entry guard + specialized body — pays the guard on every call even when the loop is cold, duplicates the whole function body instead of one loop, and the guard's O(n) first-scan can be wasted work if the loop is never reached. The loop-preheader placement already implemented is strictly better positioned.

What this PR changes

  1. Matcher (stmt/element_shape_loop.rs): admits an optional leading const r = arr[counter] element binding whose every use is a tracked r.field read. The binding's source array participates in the one-array-per-loop rule, and any other use of r — bare reference, mutable binding, non-counter index, second array — declines (each shape has a red test).
  2. Lowering: the fast clone skips the binding's Let entirely — lowering it would emit the element-read tier's calls, fail the call-free scan, and silently delete the clone (fix(gc): restore evacuation at precise safepoints — the pacing half of #7682 #7690's failure shape). r.field reads are answered by the loop fact (element_binding_local on ElementShapeLoopFact); the slow clone keeps the full body, so the side-exit protocol (re-execute the current iteration, nothing committed) is unchanged.
  3. for-of desugar (both emission sites: lower/stmt_loops.rs module-init, lower_decl/body_stmt.rs function bodies): the minted counter init is Integer(0) instead of Number(0.0) — the literal kind the integer-local collector seeds on, and the same shape a user-written let i = 0 produces. (for-in deliberately untouched.)
  4. --opt-report: the clone records a Ptr<Shape> selection — and per-read consumption — when and only when the deref block branches into the fast clone (an emitted-but-deleted clone records nothing: a gate must assert its subject was live).

Acceptance criteria, measured

  1. --opt-report on the binding case now prints Ptr<Shape> 1 selected … 1 of those selections were CONSUMED, naming the binding local; the fast clone emits no js_object_get_field_by_name_f64, no js_typed_feedback_class_field_get_guard, no js_number_coerce (asserted by IR census with the perf(repsel): the element-shape loop clone fires on arr.length and aliased element types (#7480) #7701 entered-not-just-emitted test).
  2. ✅ Case (B) untouched (static ptr_shape_elements route unchanged; full element_shape suite green — 28 tests).
  3. ✅ Soundness is tested, not argued: test-files/test_gap_repsel_element_shape_param_binding.ts runs mixed arrays, a subclass with extra fields, same-shape plain literals, a hole, mid-loop mutation, growth between calls, and a fake array-like through the parameter boundary in both spellings — byte-identical to node 26.5.1 (the pinned oracle).
  4. ⏳ Corpus rule-1 bucket: measured on the repsel: dependency JS is walled by rule 1 (unbound allocations), not containment — 506 of 746 candidates #7152/repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170 corpus with both arms; numbers in the first comment. Honest expectation set by repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's own tier data: the corpus bucket is dominated by unbound-allocation sites (40.4% nested-literal components, 16.9% opaque-runtime-callee arguments) that no read-side mechanism can serve; this PR serves the T3 (parameter-shape) slice's in-loop reads. The first-party half — the case the issue was filed on — is what moves wholesale.
  5. ⏳ Protected floors: interleaved best-of-5 on the pinned mini, outputs byte-verified against node before timing; table in the first comment.

Perf (probes, dev profile, 200k elements × 200 passes, local M1)

shape before after node
const r = ps[i]; s += r.x + r.y (param) 0.22s 0.07s 0.07s
for (const p of ps) s += p.x + p.y (param) 0.78s 0.07s 0.07s
s += ps[i].x + ps[i].y (param, already served) 0.10s 0.10s 0.07s

Known-unrelated red

large_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrier fails identically on clean main @ 423bb44 (verified in a pristine worktree); the fixture is hand-built HIR with no for…of and no element-shape loop, so it is structurally unreachable by this diff. Integration suites don't run per-PR (#5960), which is how it sits red on main.

Heads-up

#7771 (wt-7771, in flight) specializes bare a[i] fetches inside the same clone and touches the same three files — whichever lands second will need a small rebase.

Summary by CodeRabbit

  • Performance

    • Improved optimization of array-element loops passed through function parameters.
    • Added support for more array shapes and safer handling of mixed, sparse, mutated, and array-like inputs.
    • Improved integer loop-counter recognition.
  • Bug Fixes

    • Prevented optimization when indexed bindings cannot be safely verified.
    • Preserved expected error behavior for unsupported array contents.
  • Tests

    • Added extensive regression coverage for optimized and fallback execution paths.

proggeramlug pushed a commit that referenced this pull request Aug 10, 2026
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR enables call-free element-shape cloning for parameter-bound arrays. It changes synthesized for…of counters to integer locals, adds optimization-report tracking for clone selection and field-load consumption, and expands regression and GC representation-selection coverage.

Changes

Element-shape parameter binding

Layer / File(s) Summary
Integer for…of counters
crates/perry-hir/src/lower/...
Synthesized for…of counters now start with Integer(0), enabling integer-counter optimizations.
Element-shape clone selection and field loads
crates/perry-codegen/src/stmt/element_shape_loop.rs, crates/perry-codegen/src/expr/property_get/helpers.rs
The supported const r = arr[counter] form reports reachable call-free Ptr<Shape> clones. Raw-f64 field loads record consumption of the corresponding shape proof.
Clone and runtime regression coverage
crates/perry-codegen/src/stmt/element_shape_loop_tests.rs, test-files/test_gap_repsel_element_shape_param_binding.ts, test-parity/gc_repsel_corpus.txt, scripts/check_test_registration.py, changelog.d/7778-element-shape-param-binding.md
Tests verify clone entry, call-free output, counter-index restrictions, parameter arrays, mixed shapes, holes, mutations, non-array callers, escaping bindings, empty arrays, and GC registration.
Version metadata
Cargo.toml, CLAUDE.md
The workspace and documented version change from 0.5.1455 to 0.5.1456.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ForOfLowering
  participant ElementShapeLoop
  participant PropertyGetHelpers
  participant OptReport
  ForOfLowering->>ElementShapeLoop: create integer loop counter
  ElementShapeLoop->>ElementShapeLoop: verify reachable call-free clone
  ElementShapeLoop->>OptReport: record Ptr<Shape> selection
  PropertyGetHelpers->>OptReport: record PtrShape field-load consumption
Loading

Possibly related issues

Possibly related PRs

Suggested reviewers: jdalton

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The implementation addresses the matcher, lowering, reporting, soundness, regression, and benchmark requirements, but the rule-1 corpus bucket did not decrease as required by issue #7766. Provide evidence that the dependency-JS rule-1 bucket decreases from 506, or update issue #7766 acceptance criteria and scope to match the measured unchanged result.
Out of Scope Changes check ⚠️ Warning The PR includes release metadata changes in Cargo.toml, CLAUDE.md, and a changelog entry that are unrelated to the implementation and explicitly prohibited by the repository template. Remove the Cargo.toml version bump, CLAUDE.md version edit, and changelog entry; maintainers should apply release metadata changes at merge time.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: extending element-shape loop cloning to element bindings across function boundaries.
Description check ✅ Passed The description is detailed and covers scope, issue, implementation, tests, performance, soundness, and known failures despite omitting template headings and checklist items.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch repsel/7766-param-array-element-shape

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Validation results (criteria 4 + 5, plus the #7780-family hazard check)

Protected floors (criterion 5) — pinned quiet mini, interleaved best-of-5, outputs byte-verified vs node before timing

bench base fix ratio
churn 0.42 0.42 1.000
churn_alloc 0.37 0.37 1.000
push_cls 0.35 0.35 1.000
push_num 0.14 0.14 1.000
churn_read 0.02 0.02 1.000
cycles 0.19 0.19 1.000
deeplist 0.24 0.24 1.000
tree 1.63 1.63 1.000
tree_wide 2.10 2.10 1.000
retain 0.53 0.53 1.000
retain_wide 1.09 1.09 1.000
fib40 0.39 0.39 1.000

The uniform 1.000 is verified honest, not vacuous: the two arms' binaries differ (cmp on four of them), and a release-profile probe that DOES contain the changed shape separates them decisively — for…of over a parameter array: base 0.83s, fix 0.09s (9.2×), byte-identical output. None of the 12 protected benches contains a for…of or binding-form read loop (grep-verified), so identical times are the expected structural result.

Dependency-JS corpus (criterion 4) — #7152/#7170 methodology, every third __esModule module (409 selected), release binaries, both arms

Paired over the 393 modules that compiled in both arms (selection lists identical; the remaining 2/3 were 60s-timeout flakes under load):

ptr-shape bucket base fix
rule 1 (provenance) 2606 2606
rule 1 — already served by return-shape 30 30
rule 2 (containment) 42 42
rule 5 (module-wide barrier) 151 151
class admission 6 6
selected / consumed 1 / 56 1 / 56

Zero per-module differences. Two readings, both load-bearing:

  1. The dep-JS rule-1 wall does not move — as the PR body predicted from repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's tier census, it is unbound-allocation territory (nested-literal components, defineProperty descriptors, closure regions) that no read-side mechanism can serve. The first-party half — the case repsel: rule-1 provenance does not survive a function boundary — a typed P[] parameter still reads fields by name (first-party half of #7152/#7170) #7766 was filed on — is what this PR moves (to node parity). The dep-JS half stays with repsel: dependency JS is walled by rule 1 (unbound allocations), not containment — 506 of 746 candidates #7152/repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's own mechanisms.
  2. It is also a 393-real-module no-regression proof for the for-of desugar change: not one ptr-shape outcome shifted anywhere in the corpus.

(Absolute numbers are ~2× #7170's because this selection is 409 modules vs its 197 — same corpus, denser slice.)

The #7780-family hazard (third clone-deletion mode), checked here

PR #7780's session found that the element binding's lexical-death shadow-slot clear is a real js_shadow_slot_set call in the call-fallback shadow mode, which silently deletes a call-free clone. This PR's approach is structurally immune: the fast clone lowers &body[1..], so the binding never enters clear_loop_body_shadow_slots' statement walk — no clear is emitted in any mode. Verified empirically under PERRY_INLINE_SHADOW_SLOT=0 (the call-fallback mode): the deref block still cond_brs INTO the fast clone and the fast blocks contain zero calls, for both the binding and for…of spellings.

Overlap with #7780

#7780 (branch repsel/7771-element-fetch-clone) independently implements the same matcher/fact core. Unique to this PR: the for-of desugar counter fix (Integer(0) — the 9–11× for…of win), the --opt-report select/consume wiring (criterion 1), the #7766 parameter-boundary soundness gap test, and the corpus A/B. Unique to #7780: the emit_shadow_slot_clear defensive skip (moot under this PR's body-slicing but harmless belt-and-braces), and the #7775/#7776 side-find gap tests. The two need reconciling before either merges — maintainer's call which lands first; the other rebases to its unique parts.

proggeramlug added a commit that referenced this pull request Aug 10, 2026
…7782)

* fix(gc): the seeded schedule arms the poll word, like zeal (#7778)

PERRY_GC_SCHEDULE_RATE=1 saw 6 safepoints against zeal's 9,648 loop polls
on the same reproduction: nothing kept the poll word armed for the schedule
mode, so its loop-safepoint bypass sat behind a gate that never opened.
resolve_poll_seed keeps the seed when the schedule is enabled, and
ScheduleGuard mirrors ZealGuard's arm/release pair.

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

* docs: key the poll-word fix's comments to the filed issue #7781

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

* docs(changelog): fragment for the poll-word fix

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

* chore: bump version to 0.5.1451

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

* fix(gc): ScheduleGuard's arm bookkeeping must be asymmetric — disarm_poll saturates

off()-disarm-then-Drop-rearm leaks +1 permanently when the disarm lands on
a zero word (saturation loses the decrement, the paired arm does not). The
leak pinned the poll armed for the rest of the test binary: the timing test
slowed and the generation-gate contract took a safepoint drain mid-stage,
2/2 consistently. Only set() arms; only its own Drop releases. 3/3 full-suite
runs clean after.

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
@proggeramlug
proggeramlug marked this pull request as ready for review August 10, 2026 15:37
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Status: ready for review. The gap suite (scripts/run_gap_tests.sh, ~500 parity tests) is still running locally at 218/~500 and is not blocking this — the change's own parity coverage is already proven: the new test_gap_repsel_element_shape_param_binding.ts is byte-identical to node 26.5.1, and the 393-module corpus A/B found zero output or outcome differences. I will post the gap result here when it lands; if it surfaces anything, I will fix it on this branch.

proggeramlug pushed a commit that referenced this pull request Aug 10, 2026
@proggeramlug
proggeramlug force-pushed the repsel/7766-param-array-element-shape branch from 7b2267b to 4a33721 Compare August 10, 2026 17:33
Ralph Küpper added 6 commits August 10, 2026 19:36
…m through function boundaries (#7766)

The versioned clone's matcher admitted only a single-statement accumulator
body, so the element-binding spelling — 'const r = ps[i]; s += r.x' and the
for…of desugar that emits exactly that shape — kept by-name field reads
through a typed parameter array while the direct 'ps[i].x' spelling was
already served. Three changes:

1. The matcher admits a leading 'const r = arr[counter]' element binding
   whose every use is a tracked r.field read; the fast clone never
   materializes the binding (its Let is skipped — lowering it would emit
   the element-read tier's calls and silently delete the clone, #7690's
   shape), and the fact answers for both read spellings.

2. Both for-of desugars mint their counter as Integer(0) instead of
   Number(0.0) — the literal kind the integer-local collector seeds on.
   With Number(0.0) the desugared counter never joined integer_locals,
   never got a canonical i32 slot, and every i32-counter loop optimization
   silently declined the for…of spelling of loops it served in indexed form.

3. The clone records a Ptr<Shape> selection (and per-read consumption) in
   --opt-report when — and only when — the deref block cond_brs INTO the
   fast clone, so a parameter-array loop no longer reads as an unserved
   rule-1 wall.

Probes (dev profile, 200k elements x 200 passes): binding form through a
parameter 0.22s -> 0.07s, for…of 0.78s -> 0.07s — both at node parity.
Registers this PR's test_gap_repsel_element_shape_param_binding plus #7774's
test_gap_repsel_element_group_numeric, and deletes the latter's registration
exclusion: gc_repsel_matrix.sh has its own manifest pre-check that does not
consult check_test_registration.py exclusions, so the excluded fixture broke
every matrix invocation (exit 3) from the moment #7774 merged. One registry,
everything registered; the matrix's arm-level liveness gate (#7255) is the
vacuity guard for low-allocation cells.

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
@proggeramlug
proggeramlug force-pushed the repsel/7766-param-array-element-shape branch from 4a33721 to ed6be3c Compare August 10, 2026 17:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test-parity/gc_repsel_corpus.txt`:
- Line 749: Update the comment near “Low-allocation read loop” to hyphenate the
compound modifier as “zero-copying minors,” without changing the surrounding
description.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d56a647a-388b-4565-bbb0-ae4cec5b6fe2

📥 Commits

Reviewing files that changed from the base of the PR and between 4a33721 and ed6be3c.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • CLAUDE.md
  • Cargo.toml
  • scripts/check_test_registration.py
  • test-parity/gc_repsel_corpus.txt
💤 Files with no reviewable changes (1)
  • scripts/check_test_registration.py

test_gap_repsel_element_shape_param_binding

# #7770 (PR #7774, registered by #7778): the element-group numeric-field
# proof. Low-allocation read loop — its cell can run zero copying minors on

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Hyphenate the compound modifier.

Change zero copying minors to zero-copying minors to fix the grammar warning and clarify the description.

🧰 Tools
🪛 LanguageTool

[grammar] ~749-~749: Use a hyphen to join words.
Context: ...cation read loop — its cell can run zero copying minors on # a filtered arm (the ...

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-parity/gc_repsel_corpus.txt` at line 749, Update the comment near
“Low-allocation read loop” to hyphenate the compound modifier as “zero-copying
minors,” without changing the surrounding description.

Source: Linters/SAST tools

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Audit complete — merging. This PR raced #7780 (both implemented the element-binding matcher independently); the rebase keeps main's landed matcher and grafts this PR's unique delta, which is the part that actually carries the clone across function boundaries:

What survives (and was verified):

Closes #7766.

@proggeramlug
proggeramlug merged commit 762d10f into main Aug 10, 2026
1 of 18 checks passed
@proggeramlug
proggeramlug deleted the repsel/7766-param-array-element-shape branch August 10, 2026 17:51
proggeramlug pushed a commit that referenced this pull request Aug 10, 2026
…sion sites (#7766 follow-up)

#7778 made both for-of desugar sites mint the synthetic __idx_* counter as
Expr::Integer(0) — the literal kind collect_integer_let_ids seeds on, and thus
the difference between a counter that gets a canonical i32 slot and one that is
invisible to every i32-counter loop optimization.

Nothing pinned it: the desugar is correct either way and prints identical
output, so a revert to Number(0.0) would silently un-optimize every for-of loop
with no test failing. Verdict tests on the lowered HIR, one per emission site
(module-init and function-body have drifted independently before), both
sabotage-verified.
proggeramlug added a commit that referenced this pull request Aug 10, 2026
…sion sites (#7766 follow-up) (#7790)

* test(hir): pin the for-of desugar counter's literal kind at both emission sites (#7766 follow-up)

#7778 made both for-of desugar sites mint the synthetic __idx_* counter as
Expr::Integer(0) — the literal kind collect_integer_let_ids seeds on, and thus
the difference between a counter that gets a canonical i32 slot and one that is
invisible to every i32-counter loop optimization.

Nothing pinned it: the desugar is correct either way and prints identical
output, so a revert to Number(0.0) would silently un-optimize every for-of loop
with no test failing. Verdict tests on the lowered HIR, one per emission site
(module-init and function-body have drifted independently before), both
sabotage-verified.

* docs: key the changelog fragment to its own PR number

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

* chore: bump version to 0.5.1459

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repsel: rule-1 provenance does not survive a function boundary — a typed P[] parameter still reads fields by name (first-party half of #7152/#7170)

1 participant